Adding fully asynchronous versions of connect and publish. #1311
Conversation
|
Thanks @stebet! I don't want to have pull requests staying open for a long time so I'm going to fix conflicts here, review, and get it merged ASAP! |
lukebakken
force-pushed
the
asyncconnectandpublish
branch
from
77203e8 to
7076a81
Compare
Yes, this PR solves this issue where memory usage goes up the roof if the producing app is sending messages faster than they can be delivered over the network, since the output buffer is no longer unlimited. It is now limited to 128 messages and if that is full, the |
lukebakken
force-pushed
the
asyncconnectandpublish
branch
from
5ca18ac to
7059d13
Compare
Adding Timeout extension for ValueTask for connection handshake. Don't target .NET Framework 4.7.2 in Benchmarks, Test Apps and Unit Tests when not building in Windows.
…s integration environment (https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables), and add FUTURE
lukebakken
force-pushed
the
asyncconnectandpublish
branch
from
7059d13 to
63985eb
Compare
|
I'm trying to suss out the CI build failures. Works on my machine, of course. |
|
Anything left to resolve here @lukebakken? |
|
@stebet nope, just waiting around to see if anyone else will review. I just got back from some time off. Merging it! |
|
@stebet let me know what you plan to "async-ify" next (if anything, you've contributed so much so far) and I will work on another section of the code. |
There was a problem hiding this comment.
Hello,
Are you really sure about that?
It can cause deadlocks in some scenarios:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#warning-deadlocks
| var task = _frameHandler.WriteAsync(memory); | ||
| if (!task.IsCompletedSuccessfully) | ||
| { | ||
| task.AsTask().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Hello, are you really sure about that?
It can cause deadlocks in some scenarios:
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#warning-deadlocks
There was a problem hiding this comment.
We would appreciate a pull request with your ideas for improvements. Thank you.
Proposed Changes
Now that the library is using Pipelines we can create fully asynchronous versions of the RabbitMQ operations. Here is a draft PR that implements async methods to run through the connection handshake and makes an async version of BasicPublish.
TODO:
Types of Changes
Checklist
CONTRIBUTING.mddocumentFurther Comments
Going to make this a draft for now. If this makes sense, we can gradually move other operations to be async as well, and hopefully even remove the synchronous versions completely later on.